fix(ui): Trim leading and trailing whitespaces in forms #3826
Conversation
Some PUT and POST endpoints have built-in backend validation, which can cause a 400 Bad Request error in the frontend. Trimming leading and trailing whitespaces from those request bodies prevents such errors. Signed-off-by: Onur Demirci <[email protected]>
A general question here: Why don't we simply trim values in the backend, instead of requiring all frontends (UI, CLI) to trim values, and then just do a validation in the backend? |
|
Trimming the values in the backend might also an option, didn't think about it. Could that cause an other issue, when we do it in the backend? However, this PR trims the values in the frontend. |
I'm not sure. Maybe we do not want such "magic" to happen automatically, as there would be no notification to the user about this? Because usually, users probably expect values to be used exactly as entered. However, as it's currently implemented in this PR, it's still "magic", just implemented in the frontend instead of the backend. A very transparent and "non-magic" way to implement this would be to forbid the user to submit something if there are trailing spaces, and require the user to remove them by themself. So in total I see these options:
|
|
I think we can safely assume that users never intentionally want to add leading or trailing whitespace to entity names so automatic trimming in the backend should be fine. But I think in addition we should also trim those names in the frontend. |
It only trims the values when sending the request. |
Option 2b is already implemented, but maybe we could also implement option 2a? This would avoid any magic in the frontend. But I'm not sure. |
Why do both, trimming in the frontend and backend? To me, the main point of trimming in the backend is to avoid the need to do it elsewhere, to reduce code duplication that can easily go out-of-sync. |
Just for UX, but that only makes sense if the form inputs are actually updated. To me also the backend trimming would be done only for UX to avoid user frustration when accidentally adding whitespace, from a technical perspective validation is totally sufficient. |
There was a problem hiding this comment.
In here and similar places, omitting the .min(1) originally has clearly been an oversight from the UI developers, as there is no possibility for an infrastructure service to exist with an empty name. Also, a name is enforced by the backend; I can see it from the generated query client, where the name is non-nullable.
I would suggest that in all places similar to this, you replace z.string() with z.string().min(1).trim(), so trim the string, and require it to be at least 1 character after trimming the whitespaces.
There was a problem hiding this comment.
This comment is of course pending on the discussion of whether we should do everything in the back-end after all, instead of front-end.
We nowadays also have a plugin to generate Zod schemas from the OpenAPI spec, and will gradually use that in many places to partially or completely replace these manually typed form schemas throughout the UI.
Example: for creating infrastructure services, we use currently these "hand-made" form schemas:
const formSchema = z.object({
name: z.string(),
url: z.url(),
description: z.string().optional(),
usernameSecretRef: z.string(),
passwordSecretRef: z.string(),
credentialsTypes: z.array(z.enum(['NETRC_FILE', 'GIT_CREDENTIALS_FILE'])),
});but we could just import and use an auto-generated Zod schema instead:
/**
* PostInfrastructureService
*/
export const zPostInfrastructureService = z.object({
credentialsTypes: z.optional(z.array(zCredentialsType)),
description: z.optional(z.union([z.null(), z.string()])),
name: z.string(),
passwordSecretRef: z.string(),
url: z.string(),
usernameSecretRef: z.string(),
});As I said, these are all auto-generated from the OpenAPI spec, but they of course don't include features like trimming, so if back-end doesn't do it, then we anyway need to, by refining or extending (simpler) the auto-generated schema further in the UI.
There was a problem hiding this comment.
The is an example of importing the auto-generated schema and extending it in the UI:
import { zPostInfrastructureService } from '@/api/zod.gen';
const formSchema = zPostInfrastructureService.extend({
name: z.string().min(1).trim(),
});
There was a problem hiding this comment.
What confuses me here is: Shouldn't it be .string().trim().min(1) to signal the the length should be at least 1 after trimming, not before?
There was a problem hiding this comment.
I was not aware of the generated Zod schemas. Depending on how we continue with this PR, i would just import the generated schema and extend it as shown.
What confuses me here is: Shouldn't it be
.string().trim().min(1)to signal the the length should be at least 1 after trimming, not before?
Yes, it should be .strimg().trim().min(1).
But what are we doing now with this PR, do we want to have such a magic trimming of the values before sending the request, or not?
There was a problem hiding this comment.
I'm fine with it done in the front-end, at least for now.
There was a problem hiding this comment.
I have no strong opinion here right now. Please continue the review with @Etsija from my end, as it's him who still has pending requested changes, it seems.
There was a problem hiding this comment.
So from my part, it's enough for now to add the .min(1) for those fields where it doesn't make sense to allow empty strings (names, mostly), so z.string() -> z.string().trim().min(1). Once that's done, I can approve from my part.
4 participants
Some PUT and POST endpoints have built-in backend validation, which can cause a 400 Bad Request error in the frontend. Trimming leading and trailing whitespaces from those request bodies prevents such errors.